fix(security): workspace sandbox avoid time-of-check/time-of-use (TOCTOU) races#464
Conversation
…ious services and examples.
…ccessibility and add related documentation.
…d security and simplified path validation.
0x5487
changed the title
nikolasdehor
left a comment
nikolasdehor
left a comment
There was a problem hiding this comment.
Excellent security improvement — os.Root is the correct solution for workspace sandboxing, eliminating TOCTOU races and symlink escapes that string-based validation cannot fully prevent.
A few minor issues (non-blocking):
-
Missing space in error message (ReadFileTool restricted branch):
"failed to read file:file not found"→ should be"failed to read file: file not found" -
WriteFileTool inconsistent result in
executeInRootbranch: returns&ToolResult{Silent: true}with no message, while the unrestricted branch returnsSilentResult(fmt.Sprintf("File written: %s", path)). The LLM loses the path confirmation. -
ListDirTool code duplication: The restricted branch duplicates formatting logic instead of reusing
formatDirEntries().
None of these are security-critical — the core os.Root approach is sound. LGTM.
…y formatting logic, and enhance `WriteFileTool`'s result message.
|
Thanks for review. I have fixed those issues. |
…xisting files and add tests for directory creation functionality.
nikolasdehor
left a comment
nikolasdehor
left a comment
There was a problem hiding this comment.
Strong security improvement. Using os.Root (Go 1.24+) to enforce workspace boundaries at the OS level is the right approach — it eliminates the TOCTOU race conditions inherent in the old validatePath + EvalSymlinks approach.
A few observations:
-
Double validation in
getSafeRelPath— You're checking for../escapes manually before passing toos.Root, butos.Rootitself already rejects paths that escape the root. This is defense-in-depth, which is fine, but worth a comment noting it's intentional belt-and-suspenders rather than the primary security boundary. -
EditFileToolread-then-write is not atomic —root.Open()for reading, thenroot.Create()for writing has a window where the file could change. The old code had the same issue, so this isn't a regression, but worth noting for a future follow-up (e.g.,os.Root.OpenFilewithO_RDWRand in-place rewrite). -
root.Create()truncates — InEditFileTool, iff.Write()fails partway through, you'll have a truncated file. Consider writing to a temp file within the root and renaming, or at minimum documenting this risk. -
Test coverage is good — The symlink escape test, empty workspace test,
mkdirAllInRoottest, and nested directory creation test cover the important cases. The relaxed error message matching in the tests is pragmatic given platform differences. -
Code duplication — The
!t.restrictbranches duplicate the entire logic. Consider extracting the core logic (read/validate/replace/write) into a shared function that takes a reader/writer interface, reducing the surface for divergence bugs. Not a blocker though.
Overall this is a clear security win. The os.Root approach is simpler and more robust than manual path validation. LGTM.
…h host and sandboxed I/O, improving atomic writes and error handling.
|
This is incredibly valuable feedback. I’ve redesigned the solution accordingly—please take a look when you have a moment. Thanks! |
0x5487
changed the title
…face and updating tools to use it directly, removing `os.Root` dependency from `sandboxFs`.
|
Hi @0x5487 , Please fix the CI errors, thanks! |
|
@lxowalle |
|
The CI did not pass. I couldn't find the pkg/tools/registry_test.go file when I pulled your code. Please make sure you have merged the latest branch. Please run make check and make lint and try again. |
|
|
This fixes the TOCTOU race in workspace sandbox, which is a subset of the broader security hardening in #331 (shell escape bypass, symlink TOCTOU, SSRF, working_dir validation). #482 also addresses the symlink TOCTOU specifically. Suggesting consolidation to #331 which covers the widest attack surface. |
|
@0x5487 Using Go 1.24's os.Root to enforce workspace boundaries at the OS level is a really clean approach. Eliminates the TOCTOU window entirely instead of just trying to mitigate it with string checks. Nice upgrade from manual path validation. We're building a PicoClaw Dev Group on Discord for contributors to connect and collaborate. If you'd like to join, send an email to |
…TOU) races (sipeed#464) * chore: Update default host bindings from 0.0.0.0 to 127.0.0.1 for various services and examples. * config: Update default host bindings to 0.0.0.0 for improved Docker accessibility and add related documentation. * refactor: reimplement filesystem tools with `os.OpenRoot` for enhanced security and simplified path validation. * chore: revert other PR content from this branch * docs: Update Chinese README. * docs: Update Chinese README. * docs: Update Chinese README. * refactor: Reorder filesystem helper functions, extract directory entry formatting logic, and enhance `WriteFileTool`'s result message. * feat: Enhance `mkdirAllInRoot` to prevent creating directories over existing files and add tests for directory creation functionality. * Refactor filesystem tools to use a `fileReadWriter` interface for both host and sandboxed I/O, improving atomic writes and error handling. * refactor: unify filesystem read/write operations with atomic write guarantees and clearer naming. * refactor: rename `appendFileWithRW` function to `appendFile` * refactor: unify filesystem access by introducing a `fileSystem` interface and updating tools to use it directly, removing `os.Root` dependency from `sandboxFs`. * chore: run make fmt * fix: `validatePath` now returns an error when the workspace is empty.
…TOU) races (sipeed#464) * chore: Update default host bindings from 0.0.0.0 to 127.0.0.1 for various services and examples. * config: Update default host bindings to 0.0.0.0 for improved Docker accessibility and add related documentation. * refactor: reimplement filesystem tools with `os.OpenRoot` for enhanced security and simplified path validation. * chore: revert other PR content from this branch * docs: Update Chinese README. * docs: Update Chinese README. * docs: Update Chinese README. * refactor: Reorder filesystem helper functions, extract directory entry formatting logic, and enhance `WriteFileTool`'s result message. * feat: Enhance `mkdirAllInRoot` to prevent creating directories over existing files and add tests for directory creation functionality. * Refactor filesystem tools to use a `fileReadWriter` interface for both host and sandboxed I/O, improving atomic writes and error handling. * refactor: unify filesystem read/write operations with atomic write guarantees and clearer naming. * refactor: rename `appendFileWithRW` function to `appendFile` * refactor: unify filesystem access by introducing a `fileSystem` interface and updating tools to use it directly, removing `os.Root` dependency from `sandboxFs`. * chore: run make fmt * fix: `validatePath` now returns an error when the workspace is empty.
5 participants
📝 Description
Fixes path traversal vulnerability in filesystem tools.
Background:
PicoClaw agents are designed to operate within a strictly restricted "workspace" directory to prevent unauthorized access to the host filesystem. However, the previous implementation relied on manual path string validation, which was susceptible to bypasses using relative paths (e.g.,
../) or symlinks and it may still be vulnerable to time-of-check/time-of-use (TOCTOU) races, where the attacker creates a symlink after the program’s checkChanges:
This PR introduces
os.Root(new in Go 1.24) to enforce these boundaries at the operating system interaction level. By opening the workspace root once and performing all subsequent file operations relative to that root handle, we eliminate the possibility of escaping the workspace, even with malicious crafted paths.https://go.dev/blog/osroot
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
#258
📚 Technical Context (Skip for Docs)
os.Root(new in Go 1.24), which provides safer, OS-level or logical enforcement of filesystem boundaries. This prevents attacks using../or malicious symlinks to escape the workspace.executeInRoothelper, simplifying security checks and ensuring consistency across all filesystem tools (EditFileTool,WriteFileTool, etc.).🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots
☑️ Checklist